Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Multipart read_chunk without content-length #750

Merged
merged 1 commit into from
Jan 28, 2016

Conversation

tumb1er
Copy link
Contributor

@tumb1er tumb1er commented Jan 27, 2016

Allows to read body parts of multipart/form-data request without Content-Length header for parts.

Current BodyPartReader.read_chunk requires Content-Length header to be set in body part. In fact, simple html form with file field doesn't send file length (at least in Chrome), so read_chunk is unusable for large binary file uploads.

Proposed implementation does two things:

  • It reads body part chunk-by-chunk until boundary is found in two subsequent chunks.
  • If boundary is found, it is pushed back to aiohttp.streams.StreamReader via unread_data method.

Since next body part content may occure in last read chunk, BodyPartReader._unread deque is not enough, and data should be returned to StreamReader instance.


@asyncio.coroutine
def _read_chunk_from_stream(self, size):
""" Reads content chunk of body part with unknown length.
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

No need to start docstring from space.

@kxepal
Copy link
Member

kxepal commented Jan 27, 2016

Surprise to read about Chrome, but it's good to have Content-Length optional, though iirc that's not right.

@tumb1er
Copy link
Contributor Author

tumb1er commented Jan 28, 2016

@kxepal if no more comments, let's consider PR as ready-to-merge.

@kxepal
Copy link
Member

kxepal commented Jan 28, 2016

@tumb1er I think need to rebase and squash it first. There are strange and unrelated xor commits here.

@asvetlov Are you ok with?

@asvetlov
Copy link
Member

Yes, looks good to me.
@kxepal please go forward and merge when you will be ready.

@tumb1er tumb1er force-pushed the multipart_read_chunk branch from 7dda8eb to 8c7950f Compare January 28, 2016 13:54
pep8

fixes for PR

fixes for PR

fix typo

don't search boundary twice
@tumb1er tumb1er force-pushed the multipart_read_chunk branch from 8c7950f to f1351a3 Compare January 28, 2016 13:58
@tumb1er
Copy link
Contributor Author

tumb1er commented Jan 28, 2016

@kxepal squashed and rebased.

kxepal added a commit that referenced this pull request Jan 28, 2016
Multipart read_chunk without content-length
@kxepal kxepal merged commit fb2829b into aio-libs:master Jan 28, 2016
@kxepal
Copy link
Member

kxepal commented Jan 28, 2016

@tumb1er thanks!

@tumb1er tumb1er deleted the multipart_read_chunk branch January 28, 2016 14:33
This was referenced Jan 31, 2016
@lock
Copy link

lock bot commented Oct 29, 2019

This thread has been automatically locked since there has not been
any recent activity after it was closed. Please open a new issue for
related bugs.

If you feel like there's important points made in this discussion,
please include those exceprts into that new issue.

@lock lock bot added the outdated label Oct 29, 2019
@lock lock bot locked as resolved and limited conversation to collaborators Oct 29, 2019
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants